Skip to content

fix: correctly generate anyOf on unions with string and boolean constant #2208

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 8, 2025

Conversation

pixunil
Copy link
Contributor

@pixunil pixunil commented Mar 24, 2025

as far as I understood while debugging through the test code, StringType is used for string while LiteralType is used for a string constant like "s", thus StringType cannot be part of a literal union.

noticed that there is some magic regarding string which is hard to incorporate outside of LiterallUnionTypeFormatter. but it appears to me that a StringType could get discarded when it is not marked as preserve literals

closes #2207

Version

Published prerelease version: v2.5.0-next.1

Changelog

🎉 This release contains work from new contributors! 🎉

Thanks for all your work!

❤️ Valentyne Stigloher (@pixunil)

❤️ Alex (@alexchexes)

🚀 Enhancement

🐛 Bug Fix

  • fix: correctly generate anyOf on unions with string and boolean constant #2208 (@pixunil)
  • fix: fully unwrap union aliases in mapped keys to avoid generating incorrect additionalProperties #2232 (@alexchexes)
  • fix: avoid incorrect additionalProperties for Pick<..., AliasLiteralUnion> #2230 (@alexchexes)

🔩 Dependency Updates

Authors: 3

}
]
},
"var8": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure if we can use enum and type[] in the same type. Especially the combination of boolean, string, and true.

I'd bet that even if allowed, this would break in a lot of community parsers. Could we make this be a anyOf boolean const true and string enum s?

Copy link
Contributor Author

@pixunil pixunil Mar 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn’t this the analog case for var4 and var6, apart from using a number instead of boolean?
regarding whether it is allowed, at least for type and enum I don’t see a indication that they are exclusive (https://json-schema.org/draft/2020-12/draft-bhutton-json-schema-validation-01#section-6.1.1). it feels a bit redundant to specify both, as enum implicitly specifies type

@pixunil pixunil force-pushed the fix-union-string-with-constant-boolean branch from b6dcdaa to 56bdaf7 Compare March 27, 2025 22:47
@pixunil pixunil requested a review from arthurfiorette March 31, 2025 15:48
@arthurfiorette
Copy link
Collaborator

@domoritz any objection in merging this PR?

Copy link
Collaborator

@arthurfiorette arthurfiorette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code lgtm

@domoritz domoritz merged commit b07e6a3 into vega:next Jun 8, 2025
3 of 4 checks passed
Copy link

github-actions bot commented Jun 8, 2025

🚀 PR was released in v2.5.0-next.1 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Union with string and false creates wrong output
3 participants